-
Notifications
You must be signed in to change notification settings - Fork 134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Proof of concept: create an RTKQ-based React Suspense cache for the sources list #7205
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
1e93e30
to
cf0e483
Compare
cf0e483
to
0005627
Compare
✅ QA Wolf - Deploy PRHello here: 7 tests ran, see details here. ✅ 7 passed |
About to review this change set but this change would be nice to extract into its own PR (just in case it causes problems or we need to revert something for some reason).
|
export const api = createApi({ | ||
baseQuery: fakeBaseQuery(), | ||
endpoints: build => ({ | ||
getSources: build.query<SourceGroups, void>({ | ||
queryFn: async () => { | ||
const sources: newSource[] = []; | ||
|
||
// TODO Artificial delay just to see the "Loading..." for Suspense | ||
await sleep(3000); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to remove this before actually merging the changeset?
// const { data } = useGetSourcesQuery(); | ||
console.log("SourcesList trying to read data via Suspense..."); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Remove this before merging?
import { store } from "../app/store"; | ||
import { setStore } from "../features/sources/sourcesCache"; | ||
|
||
setStore(store); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be nice to avoid relying on this type of module side effect. (Current code base has a lot of those, and it complicates things like the order packages can be imported in etc.)
Maybe the SourcesList
component could grab the store from context and pass it to getSourceGroups()
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially, yeah, although this is technically a pattern we do recommend for cases where non-React-related modules need access to the store:
https://redux.js.org/faq/code-structure#how-can-i-use-the-redux-store-in-non-component-files
In theory, having it get injected from a root file should mean no import order issues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's good to avoid globals as much as possible.
Even injecting it from the (application) "root" could lead to awkward cases when writing tests for a component, for example. If we don't require/render the root, but the component uses the hook– then it's going to run into an undefined value error.
// Create or read the cache for this section of the UI (????) | ||
const sourceGroupsRecord = getCacheForType(createSourceGroupsRecord); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there's only one set of sources– one thing we need to cache per recording– and only one recording per app session– we don't really need to use getCacheForType
. We could literally just use a module-level variable to track our state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
React's (current, experimental) getCacheForType
API provides some additional functionality that we don't need in this case:
- Ability to invalidate the cache (when data changes) as part of a low-priority update – without blocking high-priority updates that might come up.
- Ability to mask a higher level cache with a lower level one. (For example, a higher level cache may be a friends list with only a few user attributes, and a lower level cache might be a user's profile details.)
In our case (1) sources are never invalidated and (2) the recording and its sources are global for an entire session.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that sense, this cache is more like the React DevTools "hook names" cache:
https://github.com/facebook/react/blob/cf665c4b73a28b034c8173f4d929205fb12d2d2e/packages/react-devtools-shared/src/hookNamesCache.js#L64-L68
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is part of where I really don't know what I'm doing with Suspense Caches :) I was attempting to mimic what I saw over in your prototype folder.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah totally makes sense. Was just sharing some thoughts as I read this but maybe it would be better for us to hop on a VC and talk about it?
// isFetching = true any time a request is in flight | ||
const isFetching = sourcesEntry!.isLoading; | ||
// isLoading = true only when loading while no data is present yet (initial load with no data in the cache) | ||
const isLoading = !hasData && isFetching; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not clear to me why sourcesEntry!.isLoading
isn't sufficient for this. RTK should reset that field to false
once it has data, no?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RTKQ differentiates between "loading", which is only true during the initial setup, and "fetching", which can go between false/true many times if you change the parameters being passed into a given query hook:
In this case, we're doing the work outside of any hook, so I actually copy-pasted this logic from inside of RTKQ's hooks code. It may not be totally necessary here, I'd have to think about it a bit more.
// isLoading = true only when loading while no data is present yet (initial load with no data in the cache) | ||
const isLoading = !hasData && isFetching; | ||
// isSuccess = true when data is present | ||
const isSuccess = sourcesEntry!.isSuccess || (isFetching && hasData); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, isFetching
and hasData
coexist? I'm confused. Doesn't data normally come after fetching completes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See above comment - for the hooks the parameter can change, and that's where I pulled this code from.
const promise = api.util.getRunningOperationPromise("getSources", undefined)!; | ||
promise.then(result => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think if the component were to re-request this data before the promise finishes, you'd end up adding multiple .then()
callbacks, wouldn't you?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm. Can that happen? Because the component is suspended and we're waiting for this exact promise to resolve before it will render again.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. React might try re-rendering in some scenarios. This is why it's important that the thing that's thrown is stable, for example. (That would also be true of any event handlers you added to that thing.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha. On the other hand, in this particular scenario, is that actually a problem?
The .then
is solely for the purpose of mutating the record with the received value, so if it runs more than once it's a no-op. Also, we're throwing the original promise, not the .then
promise.
(I was also having a bit of trouble trying to translate the sync "wakeable" logic in your example over to an async promise, which is how I ended up with this setup.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's fair.
Unless I'm misunderstanding something, I think this cache could be simplified to the following:
import { unstable_getCacheForType as getCacheForType } from "react";
import { api, SourceGroups } from "../../app/api";
import type { AppStore } from "../../app/store";
let hasPromiseListenerBeenAttached = false;
let sourceGroups: SourceGroups | null;
const selectSourceGroupsEntry = api.endpoints.getSources.select();
export function getSourceGroups(store: AppStore): SourceGroups {
if (sourceGroups !== null) {
return sourceGroups;
}
let sourcesEntry: ReturnType<typeof selectSourceGroupsEntry> | undefined =
selectSourceGroupsEntry(store.getState());
if (!sourcesEntry || sourcesEntry.isUninitialized) {
store.dispatch(api.endpoints.getSources.initiate());
sourcesEntry = selectSourceGroupsEntry(store.getState());
}
const promise = api.util.getRunningOperationPromise("getSources", undefined)!;
if (!hasPromiseListenerBeenAttached) {
hasPromiseListenerBeenAttached = true;
promise.then(result => {
sourceGroups = result.data as SourceGroups;
});
}
throw promise;
}
0005627
to
9e4dfb6
Compare
Some updates:
|
This PR:
getRunningOperationPromise
API<SourcesList>
component to read from that cache, and wraps it in a<Suspense>
+ loader (with an artificial delay added to the RTKQ API endpoint just to see that it's showing the fallback)strict: true
in thetsconfig.json
for both prototype folders, because APPARENTLY WE DIDN'T HAVE TS STRICT MODE ON AND THIS WAS BREAKING THINGS ARGHGHHHHGGH!